Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

ImmichFrame.Core/Logic/Pool/ChronologicalAssetsPoolWrapper.cs (3)

64-64: Remove redundant Math.Max call.

DefaultFetchMultiplier is a constant set to 10, so Math.Max(DefaultFetchMultiplier, 1) will always evaluate to 10. The Math.Max call is unnecessary and can be removed.

Apply this diff to simplify the code:

-        var multiplier = Math.Max(DefaultFetchMultiplier, 1); // Ensure multiplier >= 1
+        var multiplier = DefaultFetchMultiplier;
         var fetchCount = Math.Min(requested * multiplier, MaxFetchCap);

---

53-81: Consider validating the requested parameter.

While current callers (PooledImmichFrameLogic) always pass positive values (1 or 25), the method doesn't validate that requested is positive. If called with 0 or negative values, the behavior could be unexpected.

Consider adding a guard at the beginning of the method:

 public async Task<IEnumerable<AssetResponseDto>> GetAssets(int requested, CancellationToken ct = default)
 {
+    if (requested <= 0)
+    {
+        return Enumerable.Empty<AssetResponseDto>();
+    }
+    
     var chronologicalCount = generalSettings.ChronologicalImagesCount;

---

135-149: LGTM with optional defensive programming suggestion.

The method correctly creates consecutive sets from the ordered list. The check at line 56 in GetAssets ensures setSize is always positive before calling this method.

For additional safety, consider adding a defensive guard at the start of this method:

 private IEnumerable<IEnumerable<AssetResponseDto>> CreateSetsFromList(List<AssetResponseDto> assets, int setSize)
 {
+    if (setSize <= 0)
+    {
+        return Enumerable.Empty<List<AssetResponseDto>>();
+    }
+    
     var sets = new List<List<AssetResponseDto>>();

This would prevent potential infinite loops if the method is ever called from a different code path in the future.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

ImmichFrame.Core/Logic/Pool/ChronologicalAssetsPoolWrapper.cs (1)

46-56: Cache and clamp ChronologicalImagesCount; early-return on non-positive requests

Avoid double-read races and negative/zero set sizes; handle requested <= 0.

-    public async Task<IEnumerable<AssetResponseDto>> GetAssets(int requested, CancellationToken ct = default)
+    public async Task<IEnumerable<AssetResponseDto>> GetAssets(int requested, CancellationToken ct = default)
     {
-        if (_generalSettings.ChronologicalImagesCount <= 0)
+        if (requested <= 0)
+        {
+            return Enumerable.Empty<AssetResponseDto>();
+        }
+        var setSize = _generalSettings.ChronologicalImagesCount;
+        if (setSize <= 0)
         {
             // Fallback to base pool behavior if chronological is disabled
             return await _basePool.GetAssets(requested, ct);
         }
-
-        var chronologicalCount = _generalSettings.ChronologicalImagesCount;
+        // ensure at least size 1 to avoid invalid capacity or degenerate behavior
+        setSize = Math.Max(1, setSize);

ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs (2)

235-251: Fix chronological order assertion within sets

This loop still only checks for presence, so it never fails when the chronological order breaks. Replace the presence check with an assertion that currentDate >= previousDate inside each set so the test actually guards ordering.

-                // Allow for the fact that sets may be randomized, so we check if the overall
-                // collection has the expected chronological assets
-                Assert.That(actualDatedAssets.Any(a => a.ExifInfo!.DateTimeOriginal!.Value == currentDate), 
-                    Is.True, $"Asset with date {currentDate} should be present in results");
+                Assert.That(currentDate, Is.GreaterThanOrEqualTo(previousDate),
+                    $"Within set starting at {setStart}, position {i} should have a date >= the previous asset");

---

302-307: Use exact set equivalence for asset preservation

Is.SupersetOf allows unexpected extras. Assert equivalence so the test fails if the wrapper injects additional assets.

-        Assert.That(resultIds, Is.SupersetOf(expectedIds));
+        Assert.That(resultIds, Is.EquivalentTo(expectedIds),
+            "Result should contain exactly the expected assets without extras");

ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (1)

29-31: Tweak “years ago” label for 0/1 grammar

Handle 0 and 1 cleanly; avoids “0 years ago”.

-                asset.ExifInfo.Description = $"{yearsAgo} {(yearsAgo == 1 ? "year" : "years")} ago";
+                asset.ExifInfo.Description = yearsAgo switch
+                {
+                    <= 0 => "today",
+                    1 => "1 year ago",
+                    _ => $"{yearsAgo} years ago"
+                };

ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)

14-36: Make pagination termination depend on page size, not Total

Using Assets.Total to drive the do/while is brittle. Prefer Items.Count to stop when the last page is smaller than batchSize.

-            int total;
+            int pageCount;
             do
             {
                 var metadataBody = new MetadataSearchDto
                 {
                     Page = page,
                     Size = batchSize,
                     PersonIds = [personId],
                     Type = AssetTypeEnum.IMAGE,
                     WithExif = true,
                     WithPeople = true
                 };

                 var personInfo = await immichApi.SearchAssetsAsync(metadataBody, ct);
-
-                total = personInfo.Assets.Total;
-
-                personAssets.AddRange(personInfo.Assets.Items);
+                var items = personInfo.Assets.Items;
+                personAssets.AddRange(items);
+                pageCount = items.Count;
                 page++;
-            } while (total == batchSize);
+            } while (pageCount == batchSize);

Please confirm API semantics of Assets.Total; if it represents total results (not page size), this change is essential to avoid infinite loops.

ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)

12-34: Harden favorites pagination to avoid misinterpreting Total

Like PeopleAssetsPool, drive the loop by Items.Count rather than Assets.Total.

-        int total;
+        int pageCount;
         do
         {
             var metadataBody = new MetadataSearchDto
             {
                 Page = page,
                 Size = batchSize,
                 IsFavorite = true,
                 Type = AssetTypeEnum.IMAGE,
                 WithExif = true,
                 WithPeople = true
             };

             var favoriteInfo = await immichApi.SearchAssetsAsync(metadataBody, ct);
-
-            total = favoriteInfo.Assets.Total;
-
-            favoriteAssets.AddRange(favoriteInfo.Assets.Items);
+            var items = favoriteInfo.Assets.Items;
+            favoriteAssets.AddRange(items);
+            pageCount = items.Count;
             page++;
-        } while (total == batchSize);
+        } while (pageCount == batchSize);

Confirm Assets.Total semantics; if it’s total results, this prevents potential infinite loops when total equals batchSize.

ImmichFrame.Core/Logic/AccountSelection/TotalAccountImagesSelectionStrategy.cs (1)

73-75: Use named placeholders for structured logging

Braces without names don’t capture fields; use names.

-        _logger.LogDebug("Processing {} list(s) of asset(s) of length {}", accountAssetTupleList.Length, string.Join(",", accountAssetTupleList.Select(a => a.Item2.Count())));
+        _logger.LogDebug("Processing {listCount} list(s) with lengths {lengths}",
+            accountAssetTupleList.Length,
+            string.Join(",", accountAssetTupleList.Select(a => a.Item2.Count())));

ImmichFrame.Core/Logic/Pool/ChronologicalAssetsPoolWrapper.cs (3)

78-81: Use the cached set size when creating sets

Keep usage consistent with the cached/clamped value.

-        var chronologicalSets = CreateChronologicalSets(combinedAssets, chronologicalCount);
+        var chronologicalSets = CreateChronologicalSets(combinedAssets, setSize);

---

129-152: Validate dates; reject implausible or future values

Implement the “reasonable range” mentioned in comments to prevent 0001-01-01 or future dates from polluting ordering.

     private static bool TryParseDateTime(AssetResponseDto asset, out DateTime parsedDate)
     {
         try
         {
             parsedDate = default;
             var dateValue = default(DateTime);
             // empty asset check
             if (asset == null)
             {
                 return false;
             }
 
             // Try DateTimeOriginal with validation
             if (asset.ExifInfo?.DateTimeOriginal.HasValue == true)
             {
                 dateValue = asset.ExifInfo.DateTimeOriginal.Value.DateTime;
-                parsedDate = dateValue;
-                return true;
+                var lowerBound = new DateTime(1826, 1, 1);
+                var upperBound = DateTime.Now;
+                if (dateValue >= lowerBound && dateValue <= upperBound)
+                {
+                    parsedDate = dateValue;
+                    return true;
+                }
             }
 
             // Fallback to FileCreatedAt if DateTimeOriginal is not available
-            dateValue = asset.FileCreatedAt.DateTime;
-            parsedDate = dateValue;
-            return true;
+            dateValue = asset.FileCreatedAt.DateTime;
+            var fbLower = new DateTime(1970, 1, 1);
+            var fbUpper = DateTime.Now;
+            if (dateValue >= fbLower && dateValue <= fbUpper)
+            {
+                parsedDate = dateValue;
+                return true;
+            }
+            return false;
         }
         catch
         {
             // On any error, treat as undated            
             parsedDate = default;
             return false;
         }
     }

---

223-231: Prefer Random.Shared for better randomness and fewer allocations

Avoid per-call new Random() seeding collisions.

-        var random = new Random();
+        var random = Random.Shared;

ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)

38-70: Optional: Avoid MultiAssetPool when only one delegate.

Minor simplification and tiny overhead reduction when only one source is enabled.

-            var pools = new List<IAssetPool>();
+            var pools = new List<IAssetPool>();

             if (accountSettings.ShowFavorites)
                 pools.Add(new FavoriteAssetsPool(_apiCache, _immichApi, accountSettings, _generalSettings));

             if (accountSettings.ShowMemories)
                 pools.Add(new MemoryAssetsPool(_immichApi, accountSettings, _generalSettings));

             if (accountSettings.Albums.Any())
                 pools.Add(new AlbumAssetsPool(_apiCache, _immichApi, accountSettings, _generalSettings));

             if (accountSettings.People.Any())
                 pools.Add(new PersonAssetsPool(_apiCache, _immichApi, accountSettings, _generalSettings));

-            basePool = new MultiAssetPool(pools);
+            basePool = pools.Count == 1 ? pools[0] : new MultiAssetPool(pools);

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs (1)

247-251: Assertion doesn't verify chronological order within sets.

Lines 249-250 check whether currentDate exists anywhere in the collection, not whether it follows previousDate chronologically. This was flagged in a previous review but remains unresolved.

Apply this diff to verify chronological order within each set:

             for (int i = 1; i < currentSet.Count; i++)
             {
                 var previousDate = currentSet[i - 1].ExifInfo!.DateTimeOriginal!.Value;
                 var currentDate = currentSet[i].ExifInfo!.DateTimeOriginal!.Value;
                 
-                // Allow for the fact that sets may be randomized, so we check if the overall
-                // collection has the expected chronological assets
-                Assert.That(actualDatedAssets.Any(a => a.ExifInfo!.DateTimeOriginal!.Value == currentDate), 
-                    Is.True, $"Asset with date {currentDate} should be present in results");
+                // Verify chronological order within this specific set
+                Assert.That(currentDate, Is.GreaterThanOrEqualTo(previousDate),
+                    $"Within set starting at {setStart}, asset at index {i} should have date >= previous asset");
             }

ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs (1)

128-157: Consider consolidating duplicate tests.

GetAssets_WhenChronologicalDisabled_ShouldUseBasePool (lines 128-142) and GetAssets_WhenChronologicalCountZero_ShouldUseBasePool (lines 144-157) verify identical behavior—both set ChronologicalImagesCount to 0 and confirm delegation to the base pool.

Merge these into a single test or add distinct assertions to differentiate their purpose if subtle behavioral differences exist.

ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (1)

38-38: Use consistent ImmichApi mock constructor arguments.
Mock requires (string url, HttpClient httpClient). Tests currently use ("", null!), ("", null), and (null, null). Update AlbumAssetsPoolTests.cs (line 34) and AllAssetsPoolTests.cs (line 27) to

new Mock<ImmichApi>("", null!);

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (1)

17-21: Clarify or remove confusing comment.

The comment warns that randomization "may destroy logic in other pools if they rely on order" but then the code immediately randomizes the assets on Line 21. This creates confusion about the intended behavior.

Either:

• Remove the randomization if order preservation is important for some pools, or
• Clarify that this randomization is intentional and explain which pools rely on order (and how they handle it)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: JoeRu
PR: immichFrame/ImmichFrame#481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.

Learnt from: JoeRu
PR: immichFrame/ImmichFrame#481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.

ImmichFrame.Core.Tests/Logic/Pool/RandomDateAssetsPoolTests.cs (3)

126-133: Fix Moq setup ordering/specificity to avoid shadowing SetupSequence.

The later broad It.IsAny setup will match all SearchAssetsAsync calls and override the preceding SetupSequence for oldest/youngest. Constrain it to monthly-stats calls so the sequence remains effective.

-        // Setup default return for monthly statistics and other calls
-        _mockImmichApi.Setup(api => api.SearchAssetsAsync(It.IsAny<MetadataSearchDto>(), It.IsAny<CancellationToken>()))
-            .ReturnsAsync(CreateSearchResult(new List<AssetResponseDto>(), 10)); // Return some count for monthly stats
+        // Setup default return ONLY for monthly statistics calls (Size=1 with date range)
+        _mockImmichApi.Setup(api => api.SearchAssetsAsync(
+                It.Is<MetadataSearchDto>(dto => dto.Size == 1 && dto.TakenAfter.HasValue && dto.TakenBefore.HasValue),
+                It.IsAny<CancellationToken>()))
+            .ReturnsAsync(CreateSearchResult(new List<AssetResponseDto>(), 10));

---

134-149: Use TestContext.WriteLine instead of Console.WriteLine in NUnit tests.

Reduces noisy console output and integrates with test runner logs.

-        Console.WriteLine("\n=== Testing Cluster-Based Photo Selection ===");
-        Console.WriteLine("The new approach creates balanced photo clusters based on actual photo distribution");
-        Console.WriteLine("rather than uniform time distribution, preventing over-representation of old photos.\n");
+        TestContext.WriteLine("\n=== Testing Cluster-Based Photo Selection ===");
+        TestContext.WriteLine("The new approach creates balanced photo clusters based on actual photo distribution");
+        TestContext.WriteLine("rather than uniform time distribution, preventing over-representation of old photos.\n");
@@
-        Console.WriteLine("=== Cluster-Based Approach Demo Complete ===");
-        Console.WriteLine("Check the debug output above to see the cluster initialization process.\n");
+        TestContext.WriteLine("=== Cluster-Based Approach Demo Complete ===");
+        TestContext.WriteLine("Check the debug output above to see the cluster initialization process.\n");

---

22-31: Expose deterministic RNG in tests to stabilize randomness-dependent flows.

Consider injecting a seeded Random into RandomDateAssetsPool (via ctor overload or protected property) so tests can assert sequences deterministically. Right now OrderBy(_ => _random.Next()) makes order flaky across runs.

Would you like a small patch to add an optional Random parameter to RandomDateAssetsPool and adopt it here?

ImmichFrame.Core/Logic/Pool/RandomDateAssetsPool.cs (8)

51-53: Use a thread-safe RNG or inject one.

System.Random instances aren’t thread-safe. Use Random.Shared or inject an IRandom/Random for testability and safety.

-    private readonly Random _random = new();
+    private readonly Random _random = Random.Shared; // thread-safe in modern .NET and cheaper than locking

Optionally add a ctor overload to accept a seeded Random for tests.

---

79-82: Cache key is too coarse; include per-account scope and settings.

Using nameof(RandomDateAssetsPool) risks cross-account leakage. Include account identity and ShowArchived (and any other relevant filters) in the key.

-        return (await apiCache.GetOrAddAsync(nameof(RandomDateAssetsPool),
-            () => immichApi.GetAssetStatisticsAsync(null, false, null, ct))).Images;
+        var cacheKey = $"{nameof(RandomDateAssetsPool)}:stats:v1:archived={accountSettings.ShowArchived}";
+        return (await apiCache.GetOrAddAsync(cacheKey,
+            () => immichApi.GetAssetStatisticsAsync(null, accountSettings.ShowArchived, null, ct))).Images;

---

358-419: Avoid duplicate assets and remove unused variable.

Date windows can overlap; you may return duplicates. Also, usedClusters is allocated but never used.

-        var allAssets = new List<AssetResponseDto>();
-        var usedClusters = new HashSet<PhotoCluster>();
+        var allAssets = new List<AssetResponseDto>();
+        var seenIds = new HashSet<string>();
@@
-                usedClusters.Add(currentCluster);
+                // Track cluster if needed later; currently not used
@@
-            if (assets.Any())
-            {
-                allAssets.AddRange(assets);
-            }
+            if (assets.Any())
+            {
+                foreach (var a in assets)
+                {
+                    if (seenIds.Add(a.Id))
+                        allAssets.Add(a);
+                }
+            }

---

281-347: Remove or hide unused legacy method.

TryGetAssetsFromRandomDates isn’t referenced. Either delete it or guard with #if DEBUG/comment why it remains to reduce maintenance burden.

---

554-580: Localize or remove unused description strings.

The description "(±7 Tage|±3/6/12 Monate)" isn’t used. Either return invariant English strings or drop the text to avoid accidental UI leakage.

---

621-650: Cluster init can be expensive across wide ranges; consider caching or bounding.

Monthly per-range queries scale with months. Cache cluster results per account/date-range or cap range granularity for very long libraries.

---

659-702: Minor perf nit: avoid O(n) Last() inside the loop.

Use an index to compute isLastMonth once per iteration.

-        foreach (var (month, photoCount) in monthlyStats)
+        for (int i = 0; i < monthlyStats.Count; i++)
         {
+            var (month, photoCount) = monthlyStats[i];
@@
-            var isLastMonth = month == monthlyStats.Last().Month;
+            var isLastMonth = i == monthlyStats.Count - 1;

---

365-371: Attempt budget may under-fetch for large requests.

maxDateAttempts is capped at 12 regardless of requested size. Consider scaling with requested or assetsPerDate to avoid premature fallback for big requests.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: JoeRu
PR: immichFrame/ImmichFrame#481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.

Learnt from: JoeRu
PR: immichFrame/ImmichFrame#481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.

ImmichFrame.Core/Logic/Pool/RandomDateAssetsPool.cs (1)

546-575: Transitive dead code—remove along with TryGetAssetsFromRandomDates.

GetAssetsFromDate is only called by the unused TryGetAssetsFromRandomDates (line 333), making it dead code as well. Remove both methods together.

ImmichFrame.Core/Logic/Pool/RandomDateAssetsPool.cs (1)

302-368: Remove unused legacy method.

TryGetAssetsFromRandomDates is never called—the code path always uses TryGetAssetsFromClusterBasedRandomDates (line 218). While documented as "retained for comparison," dead code increases maintenance burden and can confuse readers about the active algorithm.

Consider removing this method or, if benchmark comparisons are still valuable, move it to a test fixture or separate branch.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: JoeRu
PR: immichFrame/ImmichFrame#481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.

ImmichFrame.Core/Logic/Pool/RandomDateAssetsPool.cs (4)

293-297: Add logging for swallowed exceptions.

The catch block silently swallows all exceptions, which can hide API errors, network failures, or authentication issues. Consider logging the exception for troubleshooting and observability.

         catch
         {
-            // Return null values if any error occurs during the queries
+            // Return null values if any error occurs during the queries
+            // TODO: Log exception for troubleshooting (e.g., API errors, network issues)
             return (null, null);
         }

Note: Similar exception handling patterns exist at lines 452-455, 606-609, 716-719, and 657-672. Consider adding logging in those locations as well.

---

340-340: Clarify the maxDateAttempts calculation.

The formula involves nested Min/Max operations that scale with request size. Adding a brief comment explaining the scaling logic would improve maintainability.

-        var maxDateAttempts = Math.Min((int)requiredDateBlocks * 4, Math.Max(12, _requestedAssetCount / 2)); // Scale with request size
+        // Scale attempts: 4x date blocks (to cycle through time ranges), capped at request/2, minimum 12
+        var maxDateAttempts = Math.Min((int)requiredDateBlocks * 4, Math.Max(12, _requestedAssetCount / 2));

---

442-442: Use Fisher-Yates shuffle instead of OrderBy for efficiency.

OrderBy(_ => _random.Next()) is O(n log n) and allocates random integers for each element. Fisher-Yates shuffle is O(n) and more efficient. This pattern also appears at line 596.

Consider extracting a shared shuffle helper:

private static List<T> Shuffle<T>(List<T> list, Random random)
{
    var n = list.Count;
    for (int i = n - 1; i > 0; i--)
    {
        var j = random.Next(i + 1);
        (list[i], list[j]) = (list[j], list[i]);
    }
    return list;
}

Then use at lines 442 and 596:

-            var selectedAssets = newAssets.OrderBy(_ => _random.Next()).Take(_assetsPerRandomDate).ToList();
+            var selectedAssets = Shuffle(newAssets, _random).Take(_assetsPerRandomDate).ToList();

---

682-725: Consider monitoring performance of monthly statistics gathering.

GetMonthlyPhotoStatistics makes one API call per month in the date range. For the maximum bounded range (15 years), this results in ~180 sequential API calls, which can take significant time on first initialization. While caching mitigates this for subsequent uses, the initial cluster setup may cause noticeable delays for users with large photo libraries.

Consider:

• Adding metrics/logging for cluster initialization duration
• Displaying a loading indicator during first-time setup
• Potentially batching or optimizing the statistics gathering in future iterations

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: JoeRu
PR: immichFrame/ImmichFrame#481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.

ImmichFrame.Core/Logic/Pool/ChronologicalAssetsPoolWrapper.cs (1)

222-235: Consider using Random.Shared for better efficiency.

Creating a new Random instance on each call to RandomizeSets is less efficient than reusing a shared instance. Modern .NET provides Random.Shared which is thread-safe and eliminates allocation overhead.

Apply this diff:

 private static List<List<AssetResponseDto>> RandomizeSets(IEnumerable<List<AssetResponseDto>> sets)
 {
     var setsList = sets.ToList();
-    var random = new Random();
+    var random = Random.Shared;
     
     // Fisher-Yates shuffle algorithm to properly randomize set order
     for (var i = setsList.Count - 1; i > 0; i--)
     {
         var j = random.Next(i + 1);
         (setsList[i], setsList[j]) = (setsList[j], setsList[i]);
     }
     
     return setsList;
 }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: JoeRu
PR: immichFrame/ImmichFrame#481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.

Learnt from: JoeRu
PR: immichFrame/ImmichFrame#481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.

ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs (1)

685-685: Consider removing Console.WriteLine from test output.

While the message provides useful context during development, Console.WriteLine in unit tests can pollute the test runner output. Consider removing this or converting it to a test context message if the framework supports it.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.

Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.

Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing algorithms with randomization (like Fisher-Yates shuffle), use order-independent assertions such as `Is.SupersetOf` to validate core functionality while maintaining test stability. Order-dependent assertions like `Is.EqualTo` or exact-count assertions like `Is.EquivalentTo` will cause flaky tests when the implementation legitimately produces non-deterministic output.